NativeEngine: texture upload, cubemap loading, and readback improvements - #1808

[Reviewed by Copilot on behalf of @bghgary]

CloneLeaf returns nullptr for anything that is not a symbol or a constant, and the flip is then skipped silently. texelFetch(s, coord, someComputedLod) samples with un-flipped Y and produces a wrong image with no diagnostic. Worth throwing or falling back rather than returning the coordinate unchanged.

Confirmed, and fixed in a3241c1 — CloneLeaf is gone, replaced by CloneExpression, a structural deep clone that handles symbols, constant unions, binary, unary and aggregate nodes. It copies each node's operator, type and source location verbatim rather than rebuilding through TIntermediate::add*, so no constant folding or type promotion can make the copy diverge from the original. Anything it genuinely cannot copy now throws std::runtime_error instead of returning nullptr, so we can never again silently emit an unflipped coordinate.

Two notes from checking how much this actually mattered:

I surveyed the 133 texelFetch call sites in the Babylon.js shaders. lod is almost always a constant (0, 1) or a bare symbol, so the old leaf-only clone was usually enough for it. But coordinates are frequently complex — ivec2(gl_FragCoord.xy), ivec2(vUV * texSize), coord + ivec2(1,1), ivec2(t0 % w, t0 / w) are all common. So the nullptr path was not an exotic corner; the reason it did not show up as failures is the second point below.

One behaviour I want to flag explicitly rather than have it found later: cloning an EOpFunctionCall aggregate duplicates a side-effecting call. That is not a regression — it is exactly what the ProcessSamplerFlip macro this code replaced already did, since it expanded the coordinate operand twice — but it is inherent to expressing the flip as ivec2(c.x, size.y - 1 - c.y) and worth knowing about.

[Reviewed by Copilot on behalf of @bghgary]

sampler and lod are cloned because reusing a node would give it two parents, but coordinate is referenced twice right here — once for .x on the line above, once for .y here — and is not cloned. By the argument in the comment at the top of this function, that is the same aliasing: the original reference is replaced by the returned aggregate, but two new ones are created in its place.

Is the coordinate safe for a reason the sampler and lod are not, or should it be cloned too?

You were right, and thank you for pushing on it — the answer to "was the coordinate safe for a different reason?" is no, it was not safe at all. Fixed in a3241c1: the coordinate is now cloned too, with the original node supplying one reference and the clone the other, so every node ends up with exactly one parent.

The post-visit ordering only masked one symptom (a nested texelFetch being flipped twice). The aliased DAG still went on to sampler splitting and SPIR-V generation, which is precisely the situation the comment right above cites as the reason for cloning sampler and lod. I have updated that comment so it now explains why all three operands are cloned instead of implying the coordinate is special.

For coverage, I extended ShaderCompilation.CompileComprehensiveGLSL rather than relying on "the render suite still passes". That distinction turned out to matter: I instrumented the traverser and ran the full 301-test validation suite, and it only hit three texelFetch flips, all of them with a bare symbol coordinate and a constant lod — i.e. the suite gave the new code zero coverage. The test now exercises a constructor, a binary expression, a nested constructor over a float expression, a nested constructor over integer binaries, built-in calls, and a non-constant lod, which cover the unary, binary and aggregate clone paths and a symbol lod. Verified by the same instrumentation that all of those shapes now reach CloneExpression.

While writing that test I hit two unrelated constructs that the WEBMIN builds reject — integer multiply (the OpIMul case already called out in this PR) and bitwise &. I kept them out of the test and left a comment saying why, so nobody re-adds them and blames the flip.

[Reviewed by Copilot on behalf of @bghgary]

m_depth is only ever assigned here, and Dispose() clears just the handle — so a Texture re-created as 2D or cube after having been 3D keeps the old depth and Depth() returns a stale value. The sibling Create* methods each reset m_is3D but none of them reset m_depth.

Each Create* hand-assigning its own subset of the metadata is what made the dead m_is3D store possible too; a shared reset would close both.

Confirmed and fixed in a3241c1. m_depth was assigned only by Create3D, and Dispose() clears just the handle, so a Texture re-created as 2D or cube after having been 3D kept reporting the old depth.

I took the shared-reset route you suggested rather than adding one more hand-written assignment. There is now a private ResetMetadata() that returns every shape field to its default, and Create2D, Create3D, CreateCube and Attach all call it immediately after Dispose() before assigning the subset that applies to them. That also let me delete the scattered m_isCube = false; m_is3D = false; lines, which is what made the class of bug possible in the first place: any field a given path forgets is now defaulted rather than inherited from the previous, differently shaped texture.

[Reviewed by Copilot on behalf of @bghgary]

requiredSize / height is only a row stride for uncompressed formats. Measured with bgfx::calcTextureSize on this branch (Win32 D3D11, RelWithDebInfo):

So for a 4x4 BC1 the loop makes 4 passes of 2 bytes over a single 8-byte block, reversing the [color0:2][color1:2][indices:4] layout into garbage; at 16x16 it reverses 16 8-byte chunks where the real layout is 4 block-rows of 32. Only the uncompressed row matches.

Reachable by default rather than in principle: bgfx::getCaps()->originBottomLeft is 0 on D3D11 (bgfx logs origin top left), so flip = originBottomLeft ? invertY : !invertY is true whenever invertY is left at its default.

bgfx::updateTexture2D also needs block-aligned x/y/width/height for compressed textures, which is not checked either. Rejecting compressed formats here would be enough, given the base upload path already handles them.

Confirmed, and your arithmetic is right. I checked the block info in bimg (image.cpp:30, {4, 4, 4, 8, ...} for BC1 — a 4x4 block in 8 bytes): at 4x4 requiredSize / height gives 8/4 = 2 against a real block row of 8, and at 16x16 it gives 128/16 = 8 against a correct 32.

It is also reachable by default rather than theoretical: originBottomLeft is only ever set true in renderer_gl.cpp, so on D3D11 it is 0 and flip = !invertY is true whenever invertY is left false.

Fixed in a3241c1 by rejecting block-compressed formats outright, as you suggested, rather than fixing the stride. Fixing the stride would not be enough: mirroring block rows cannot flip a texture vertically without re-encoding the texel rows packed inside each block. BC1 happens to keep its 4 index bytes in row order, but BC3 alpha and BC7 do not, so a correct flip means a full decode/re-encode. bgfx also wants block-aligned x/y/width/height here, which the code was not checking either; rejecting the formats moots that too.

The base upload path (loadTexture → PrepareImage) already handles compressed data, so this is not a capability loss, and the error message says so. The check is texture->Format() < bgfx::TextureFormat::Unknown, which works because every block-compressed format sorts before Unknown in bgfx's enum (bgfx.h:222, "Compressed formats above"). That keeps it bimg-free, which matters here since this file has to build in configurations where bimg is not linked.

This deep clone regresses MacOS_Sanitizers: it fails on this commit and passes on both 5d8c2e1a and master aa244ec9.

intermediate.h:1707: runtime error: load of value 190, which is not a valid value for type 'bool'
  isUserDefined() <- CloneExpression:1931 <- CloneExpression:1906 <- FlipVerticalTexelCoordinate:1846

glslang initializes userDefined in TIntermAggregate() but not in TIntermAggregate(TOperator), so clones built here carry it uninitialized (190 = 0xBE fill). Default-constructing and setting the operator afterwards fixes that:

The read at 1931 still needs handling — it dereferences the same member on the source aggregate, so copying it unconditionally would inherit the same garbage.

PrepareImage can now return nullptr, but only the loadTexture call site checks it (L1650). The other three pass the result straight into an unconditional dereference: L1712 into LoadTextureFromImage (L334), and L2175 / L2222 into LoadCubeTextureFromImages (L368). Guard those three, or have PrepareImage throw instead of returning nullptr.

The two other conversions in this function are unchecked as well: the sRGB imageConvert at L284 falls through to image->m_format at L299, and the imageGenerateMips result at L323 is returned without a check.